Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Communicate process kill signals without being blocked by stdout/err reads #472

Merged
merged 14 commits into from
Mar 31, 2025
Merged

Communicate process kill signals without being blocked by stdout/err reads #472

merged 14 commits into from
Mar 31, 2025

Conversation

0div
Copy link
Contributor

@0div 0div commented Mar 19, 2025 •
edited
Loading

Description

  • use go channels to communicate signals to the handler read loops in envd and broadcast shutdown w/o being blocked by blocked reads
  • make local envd more similar to what provision.sh does in template-manager

Test

I will commit an integration test in E2B js-sdk that goes like this:

test.skipIf(!isIntegrationTest)(
  'kill running nextjs app and make sure process is gone',
  async () => {
    const sandbox = await Sandbox.create(integrationTestTemplate, {
      timeoutMs: 60_000,
    })

    const cmd = await sandbox.commands.run('cd /test-app && next dev', {
      background: true,
    })

    await wait(30_000)

    const pid = cmd.pid
    console.log('PID', pid)

    await sandbox.commands.kill(pid)
    const processes = await sandbox.commands.list()
    console.log('processes:', processes)
    assert.equal(processes.length, 0)
  }
)

@0div 0div added the bug Something isn't working label Mar 19, 2025
@0div 0div self-assigned this Mar 19, 2025
@linear Linear
Copy link

linear bot commented Mar 19, 2025

E2B-1790 Killing command by PID may not work as expected

@dobrac
Copy link
Contributor

dobrac commented Mar 19, 2025

await sandbox.commands.kill(pid)
await wait(10_000)

is it necessary to wait after the kill? Shouldn’t the await be enough?

@mlejva
Copy link
Member

mlejva commented Mar 19, 2025 •
edited
Loading

await sandbox.commands.kill(pid)
await wait(10_000)

Agree with @dobrac here. Why to have wait here if we're already awaiting kill?

@0div
Copy link
Contributor Author

0div commented Mar 19, 2025

await sandbox.commands.kill(pid)
await wait(10_000)

Agree with @dobrac here. Why to have wait here if we're already await kill?

@dobrac @mlejva correct it is not necessary, this is a remnant of older test iterations, i haven't committed this code yet.

@mlejva
Copy link
Member

mlejva commented Mar 19, 2025

@0div just a quick question, would it make sense to add tests for this? Or it makes more sense to have the tests in the SDK repo?

@0div
Copy link
Contributor Author

0div commented Mar 19, 2025

@0div just a quick question, would it make sense to add tests for this? Or it makes more sense to have the tests in the SDK repo?

@mlejva as a unit test? it would be difficult to emulate linux syscall signal behaviour.
in this repo's integration test? i could move the sdk test into that, yes.

@mlejva
Copy link
Member

mlejva commented Mar 19, 2025 •
edited
Loading

@0div just a quick question, would it make sense to add tests for this? Or it makes more sense to have the tests in the SDK repo?

@mlejva as a unit test? it would be difficult to emulate linux syscall signal behaviour. in this repo's integration test? i could move the sdk test into that, yes.

I suppose more of an integration test. It would also help with reviewing the PR. I can approve it and sort of see it's working by looking at the code but the test would make this easier

@dobrac Linear
Copy link
Contributor

dobrac commented Mar 19, 2025 •
edited
Loading

@0div there is no setup for envd client in the integration tests (running command, killing command inside of the sandbox), I can try to set it up so you can take the test to the infra

@0div
Copy link
Contributor Author

0div commented Mar 19, 2025

@0div there is no setup for envd client in the integration tests (running command, killing command inside of the sandbox), I can try to set it up so you can take the test to the infra

@dobrac it would be cool to be able to move/adapt these SDK integration-y tests (at least the setup and template) into this!

@ValentaTomas
Copy link
Member

Added an alternative implementation building on top of #480 that we should probably use here.

@0div
Copy link
Contributor Author

0div commented Mar 24, 2025

Added an alternative implementation building on top of #480 that we should probably use here.

Should I merge it into here?

@ValentaTomas ValentaTomas removed the request for review from jakubno March 25, 2025 00:53
@0div 0div merged commit 6b14db4 into main Mar 31, 2025
10 checks passed
@0div 0div deleted the killing-command-by-pid-may-not-work-as-expected-e2b-1790 branch March 31, 2025 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@r33drichards r33drichards r33drichards left review comments

@ValentaTomas ValentaTomas ValentaTomas approved these changes

@dobrac dobrac Awaiting requested review from dobrac dobrac is a code owner

Assignees

@0div 0div

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@0div @dobrac @mlejva @ValentaTomas @r33drichards